cmd/ctrlc/root/sync/aws/common/provider.go (2)

10-12: Incomplete function documentation.

The function documentation comment starts with "The" but doesn't complete the sentence.

// ComputeProviderDetails generates a provider name and region string based on the provided parameters.
-// The
+// The function modifies the name pointer to hold the computed provider name.

---

17-19: Check for empty regions slice.

When checking if regions are provided, the code checks both regions != nil and len(regions) > 0. Since go slices are never nil after initialization but may be empty, you could simplify this check.

	// Use regions for name if none provided
-	if regions != nil && len(regions) > 0 {
+	if len(regions) > 0 {
		providerRegion = strings.Join(regions, "-")
	}

cmd/ctrlc/root/sync/azure/common/resourceGroup.go (1)

33-38: Consider checking for nil properties in resource group objects.

Azure SDK might return resource group objects with nil Name or Location properties in exceptional cases. Consider adding nil checks before dereferencing.

		for _, rg := range page.Value {
+			// Skip resource groups with nil properties
+			if rg.Name == nil || rg.Location == nil {
+				continue
+			}
			results = append(results, ResourceGroupInfo{
				Name:     *rg.Name,
				Location: *rg.Location,
			})
		}

cmd/ctrlc/root/sync/aws/aws.go (1)

19-26: Consider updating command examples.

With the addition of the networks sync command, it would be helpful to update the example documentation to include a networks example.

		Example: heredoc.Doc(`
			# Make sure AWS credentials are configured via environment variables or ~/.aws/credentials
			
			# Sync all EC2 instances from a region
			$ ctrlc sync aws ec2 --region us-west-2
			
			# Sync EC2 instances from a region every 5 minutes
			$ ctrlc sync aws ec2 --region us-west-2 --interval 5m
+
+			# Sync VPC networks from a region
+			$ ctrlc sync aws networks --region us-west-2
		`),

cmd/ctrlc/root/sync/aws/networks/networks.go (2)

21-31: Doc-strings & comments reference Google – copy-paste error

Several comments still say “Google Networks / application default credentials / Compute Engine”.
This is misleading for maintainers.

-// NewSyncNetworksCmd creates a new cobra command for syncing Google Networks
+// NewSyncNetworksCmd creates a new cobra command for syncing AWS VPC networks
...
-			# Make sure AWS credentials are configured via environment variables or application default credentials
+			# Make sure AWS credentials are configured via environment variables or AWS CLI

---

408-431: Resource-provider relationship rules omitted

Unlike the EKS sync, network resources are not linked to relationship rules (e.g. VPC → Subnet).
Consider adding them before the first upsert to preserve graph integrity.

cmd/ctrlc/root/sync/azure/networks/networks.go (4)

208-211: Log message mentions “clusters” instead of “networks”

Minor wording mismatch that can confuse operators when reading logs.

-    log.Warn("Some clusters failed to sync", "errors", len(syncErrors))
+    log.Warn("Some networks failed to sync", "errors", len(syncErrors))

---

294-303: Incorrect azure/resource-type value for virtual-network metadata

The resource type for a VNet should be Microsoft.Network/virtualNetworks, not .../virtualNetworks/subnets.

-    "azure/resource-type":  "Microsoft.Network/virtualNetworks/subnets",
+    "azure/resource-type":  "Microsoft.Network/virtualNetworks",

---

342-349: Azure Portal URL missing tenant & subnet name

1. Azure Portal URLs conventionally include the tenant segment (#@TENANT_ID/…).
2. Subnet URLs should end with /subnets/<subnetName> so the user lands on the specific subnet.

Example fix for both helpers:

-func getVirtualNetworkConsoleUrl(resourceGroup, subscriptionID, networkName string) string {
+func getVirtualNetworkConsoleUrl(resourceGroup, subscriptionID, tenantID, networkName string) string {
     return fmt.Sprintf(
-        "https://portal.azure.com/#@/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s",
-        subscriptionID,
-        resourceGroup,
-        networkName,
+        "https://portal.azure.com/#@%s/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s",
+        tenantID,
+        subscriptionID,
+        resourceGroup,
+        networkName,
     )
 }
 
-func getSubnetConsoleUrl(resourceGroup, subscriptionID, networkName string) string {
+func getSubnetConsoleUrl(resourceGroup, subscriptionID, tenantID, networkName, subnetName string) string {
     return fmt.Sprintf(
-        "https://portal.azure.com/#@/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets",
-        subscriptionID,
-        resourceGroup,
-        networkName,
+        "https://portal.azure.com/#@%s/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets/%s",
+        tenantID,
+        subscriptionID,
+        resourceGroup,
+        networkName,
+        subnetName,
     )
 }

Call-sites will need to pass the new parameters.

---

365-373: Duplicate provider name defaulting

runSync already assigns a default when the name is empty; repeating the same check here is redundant.
Consider removing one of the two blocks to avoid drift.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

cmd/ctrlc/root/sync/aws/common/provider.go (1)

27-27: ⚠️ Potential issue

Potential panic with empty regions slice.

The function tries to access regions[0] without checking if the slice has any elements, which could cause a panic if regions is empty.

-		cfg, err := InitAWSConfig(ctx, regions[0])
+		// Default to first region, but handle empty regions case
+		region := "us-east-1" // Default region
+		if len(regions) > 0 {
+			region = regions[0]
+		}
+		cfg, err := InitAWSConfig(ctx, region)

cmd/ctrlc/root/sync/azure/networks/networks.go (2)

328-333: ⚠️ Potential issue

Same nil-pointer pitfall for subnet provisioning state

subnet.Properties and subnet.Properties.ProvisioningState need checks before dereferencing.

-        "azure/status": func() string {
-            if network.Properties != nil {
-                return string(*subnet.Properties.ProvisioningState)
-            }
-            return ""
-        }(),
+        "azure/status": func() string {
+            if subnet.Properties != nil && subnet.Properties.ProvisioningState != nil {
+                return string(*subnet.Properties.ProvisioningState)
+            }
+            return ""
+        }(),

---

382-385: ⚠️ Potential issue

getSubnetState mirrors the same issue – add nil guards

-func getSubnetState(subnet *armnetwork.Subnet) string {
-    if subnet.Properties != nil {
-        return string(*subnet.Properties.ProvisioningState)
-    }
-    return ""
+func getSubnetState(subnet *armnetwork.Subnet) string {
+    if subnet.Properties != nil && subnet.Properties.ProvisioningState != nil {
+        return string(*subnet.Properties.ProvisioningState)
+    }
+    return ""
 }

cmd/ctrlc/root/sync/aws/common/provider.go (4)

10-12: Fix incomplete function documentation.

The comment for EnsureProviderDetails begins with "The" but doesn't complete the sentence. This makes it difficult for users of this function to understand its full purpose and behavior.

// EnsureProviderDetails generates a provider name and region string based on the provided parameters.
-// The
+// The function attempts to retrieve the AWS account ID to create a detailed provider name.
+// If unable to retrieve the account ID, it falls back to a simpler naming format.

---

17-19: Simplify the condition by removing unnecessary nil check.

In Go, len() is safely defined for nil slices and returns 0, making the nil check redundant.

-	if regions != nil && len(regions) > 0 {
+	if len(regions) > 0 {
		providerRegion = strings.Join(regions, "-")
	}

🧰 Tools

🪛 golangci-lint (1.64.8)

17-17: S1009: should omit nil check; len() for nil slices is defined as zero

(gosimple)

---

12-14: Consider returning the provider name instead of modifying a pointer.

The function currently modifies the input name pointer instead of returning the computed value. This makes the function harder to test and use, and is generally considered less idiomatic in Go.

func EnsureProviderDetails(
	ctx context.Context, prefix string, regions []string, name *string,
-) {
+) string {
+	// If name is already provided and not empty, return it
+	if name != nil && *name != "" {
+		return *name
+	}
+
 	providerRegion := "all-regions"
 	// Use regions for name if none provided
 	if len(regions) > 0 {
 		providerRegion = strings.Join(regions, "-")
 	}

-	// If name is not provided, try to get account ID to include in the provider name
-	if name == nil {
-		name = new(string)
-	}
-	if *name == "" {
+	// Try to get account ID to include in the provider name
 		// Get AWS account ID for provider name using common package
 		region := "us-east-1" // Default region
 		if len(regions) > 0 {
 			region = regions[0]
 		}
 		cfg, err := InitAWSConfig(ctx, region)
 		if err != nil {
 			log.Warn("Failed to load AWS config for account ID retrieval", "error", err)
-			*name = fmt.Sprintf("%s-%s", prefix, providerRegion)
+			return fmt.Sprintf("%s-%s", prefix, providerRegion)
 		} else {
 			accountID, err := GetAccountID(ctx, cfg)
 			if err == nil {
 				log.Info("Retrieved AWS account ID", "account_id", accountID)
-				*name = fmt.Sprintf("%s-%s-%s", prefix, accountID, providerRegion)
+				return fmt.Sprintf("%s-%s-%s", prefix, accountID, providerRegion)
 			} else {
 				log.Warn("Failed to get AWS account ID", "error", err)
-				*name = fmt.Sprintf("%s-%s", prefix, providerRegion)
+				return fmt.Sprintf("%s-%s", prefix, providerRegion)
 			}
 		}
-	}
 }

Note: This refactoring would require updating all call sites to use the returned value instead of passing a pointer.

---

31-40: Duplicate error handling logic can be simplified.

The same fallback provider name pattern is used in both error cases. This can be simplified to reduce duplication.

		if err != nil {
			log.Warn("Failed to load AWS config for account ID retrieval", "error", err)
-			*name = fmt.Sprintf("%s-%s", prefix, providerRegion)
		} else {
			accountID, err := GetAccountID(ctx, cfg)
			if err == nil {
				log.Info("Retrieved AWS account ID", "account_id", accountID)
				*name = fmt.Sprintf("%s-%s-%s", prefix, accountID, providerRegion)
-			} else {
-				log.Warn("Failed to get AWS account ID", "error", err)
-				*name = fmt.Sprintf("%s-%s", prefix, providerRegion)
+				return
			}
+			log.Warn("Failed to get AWS account ID", "error", err)
		}
+		*name = fmt.Sprintf("%s-%s", prefix, providerRegion)

cmd/ctrlc/root/sync/azure/networks/networks.go (1)

209-212: Misleading log message – “clusters” vs “networks”

log.Warn("Some clusters failed to sync" …) was probably copied from another command. Replace clusters with networks to keep logs accurate.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

cmd/ctrlc/root/sync/azure/networks/networks.go (3)

30-41: Fix incorrect terminology in the examples.

The examples incorrectly mention "AKS VPCs and subnets" which doesn't match the command's purpose. This command syncs Azure Virtual Networks, not specifically AKS (Azure Kubernetes Service) resources.

-			# Sync all AKS VPCs and subnets from the subscription
+			# Sync all Azure Virtual Networks and subnets from the subscription
 			$ ctrlc sync azure networks
 			
-			# Sync all AKS VPCs and subnets from a specific subscription
+			# Sync all Azure Virtual Networks and subnets from a specific subscription
 			$ ctrlc sync azure networks --subscription-id 00000000-0000-0000-0000-000000000000
 			
-			# Sync all AKS VPCs and subnets every 5 minutes
+			# Sync all Azure Virtual Networks and subnets every 5 minutes
 			$ ctrlc sync azure networks --interval 5m

---

193-200: Error handling stops processing other networks in the same resource group.

When a single network fails to process, the entire goroutine returns, skipping all other networks in the same resource group. Consider continuing to process other networks even if one fails.

 					resources, err := processNetwork(ctx, network, resourceGroup, subscriptionID, tenantID)
 					if err != nil {
 						log.Error("Failed to process network", "name", *network.Name, "error", err)
 						mu.Lock()
 						syncErrors = append(syncErrors, fmt.Errorf("network %s: %w", *network.Name, err))
 						mu.Unlock()
-						return
+						continue  // Continue with other networks instead of returning
 					}

---

333-337: Redundant nil check before len() operation.

In Go, len() is safe to call on nil slices and will return 0, so the nil check is unnecessary.

-	if subnet.Properties != nil {
-		if subnet.Properties.PrivateEndpoints != nil && len(subnet.Properties.PrivateEndpoints) > 0 {
+	if subnet.Properties != nil {
+		if len(subnet.Properties.PrivateEndpoints) > 0 {
 			privateAccess = true
 		}
 	}

🧰 Tools

🪛 golangci-lint (1.64.8)

334-334: S1009: should omit nil check; len() for nil slices is defined as zero

(gosimple)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

cmd/ctrlc/root/sync/azure/networks/networks.go (2)

224-236: ⚠️ Potential issue

Potential nil-pointer dereference when building the resource object

network.Name, network.ID, and other pointer fields are not guaranteed to be non-nil. Any of them being nil will cause a panic.

Add defensive checks:

-	networkName := network.Name
+	if network.Name == nil || network.ID == nil {
+		return nil, fmt.Errorf("network object missing Name or ID")
+	}
+	networkName := network.Name

// ...

-		Name:       *networkName,
-		Identifier: *network.ID,
+		Name:       *networkName,
+		Identifier: *network.ID,

---

244-247: ⚠️ Potential issue

Potential nil-pointer dereference for network.Type and network.Location

Both network.Type and network.Location are pointers returned by the SDK and are not guaranteed to be non-nil.

Add safety checks:

-				"type":        network.Type,
-				"region":      network.Location,
+				"type":        func() *string {
+					if network.Type != nil {
+						return network.Type
+					}
+					return nil
+				}(),
+				"region":      func() *string {
+					if network.Location != nil {
+						return network.Location
+					}
+					return nil
+				}(),

cmd/ctrlc/root/sync/azure/networks/networks.go (5)

334-335: Simplify nil slice check

The len() function for nil slices is defined as zero, so the nil check is redundant.

-		if subnet.Properties.PrivateEndpoints != nil && len(subnet.Properties.PrivateEndpoints) > 0 {
+		if len(subnet.Properties.PrivateEndpoints) > 0 {
			privateAccess = true
		}

🧰 Tools

🪛 golangci-lint (1.64.8)

334-334: S1009: should omit nil check; len() for nil slices is defined as zero

(gosimple)

---

212-213: Fix logging message

The warning message refers to "clusters" but this command syncs networks, not clusters.

-		log.Warn("Some clusters failed to sync", "errors", len(syncErrors))
+		log.Warn("Some networks failed to sync", "errors", len(syncErrors))

---

33-34: Fix typo in example documentation

The example documentation refers to "AKS VPCs" but this command syncs Virtual Networks.

-			# Sync all AKS VPCs and subnets from the subscription
+			# Sync all Azure Virtual Networks and subnets from the subscription

---

36-37: Fix typo in example documentation

Similar typo in another example.

-			# Sync all AKS VPCs and subnets from a specific subscription
+			# Sync all Azure Virtual Networks and subnets from a specific subscription

---

39-40: Fix typo in example documentation

Similar typo in the interval example.

-			# Sync all AKS VPCs and subnets every 5 minutes
+			# Sync all Azure Virtual Networks and subnets every 5 minutes

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro